feat: add limits to input attributes by kdrienCG · Pull Request #4063 · GEOS-DEV/GEOS

kdrienCG · 2026-05-20T09:02:57Z

This PR adds the possibility to specify a min and/or a max value to Wrappers attributes.

The goal is to provide the foundations for two follow-up efforts planned in future PRs:

applying this limit feature wherever GEOS has known, constant ranges that can be validated;
extending the generated documentation (e.g. with additional text or a dedicated column) to display the limits associated with each attribute.

Usage:

registerWrapper( viewKeysStruct::fooString(), &m_foo )
  .setLimits( 0.0, 1.0 )  // sets a minimal value of 0.0 and a maximal value of 1.0

This will serve as a documentation for values, but also prevents users from making typos that could lead to wrong results, taking time to find (if noticed).

The limits come with 3 modes:

Indicative: No validation of the input is made, serves as documentation only.
Warning: A warning is emitted if the attribute's value is outside the allowed range.
Error: An error is emitted if the attribute's value is outside the allowed range.

The mode can be set when specifying the limit, using this syntax:

  .setLimits( 0.0, 1.0, LimitsMode::Warning )

... or it will default to Error if unset.

Limits also support inclusive and exclusive bounds:

  .setLimits( inclusive( 0.0 ), exclusive( 1.0 ) )

Not using inclusive(...) or exclusive(...) will default to an inclusive limit.

When using LimitMode::Error, the following output is produced when an input value exceeds a limit:

***** Exception
***** LOCATION: /.../GEOS/src/coreComponents/dataRepository/Wrapper.hpp:1245
***** Rank 0
***** Message from relperm/phaseMinVolumeFraction (/.../GEOS/inputFiles/compositionalMultiphaseFlow/deadoil_3ph_baker_1d.xml, l.112):
Value 40.05 is outside the allowed range [0, 1].

Some examples:

  .setLimits( 0.0, 1.0 )  // [0.0, 1.0]
  .setLimits( 0.0, std::nullopt )  // [0.0, +inf)
  .setLimits( inclusive( 0.0 ), std::nullopt )  // [0.0, +inf)
  .setLimits( std::nullopt, exclusive( 0.0 ) )  // (-inf, 0.0)

Will not compile due to std::optional instantiation (used by m_minValue and m_maxVavlue) for abstract types like PartitionBase

setLimits() has the same capabilities and should be the only one kept.

dkachuma

I really like this idea. It will simplify greatly some of the boiler plate validation code.

Will this work for lists of numbers?

This part may be refactored, to build the range string somewhere else

MelReyCG

Please apply this feature on at least one (maybe basic) 'Group' to demonstrate usage (array and scalar ideally).

MelReyCG · 2026-06-22T10:39:20Z

+   *                           // default to true.
+   * @endcode
+   */
+  Bound( T v, bool inclusive = true )


do we want this constructor explicit?

It was made not explicit so we can write .setLimits( 0.0, 1.0 ) instead of .setLimits( WrapperBound{ 0.0 }, WrapperBound{ 1.0 } )

That implicit conversion from any double to your type, at this (critical) level of the inclusion hierarchy, may cause the compiler to try undesired casts.
Why not an overload or a different signature for setLimits()?

kdrienCG · 2026-06-23T12:54:07Z

Please apply this feature on at least one (maybe basic) 'Group' to demonstrate usage (array and scalar ideally).

Applied to BrooksCoreyBakerRelativePermeability that uses arrays in commit 931c469

MelReyCG · 2026-06-23T13:40:56Z

+    string const msg = GEOS_FMT( "Value {} for attribute '{}' is outside the allowed range {}.",
+                                 value, getDataContext(), m_limits.getRangeStr() );
+
+    switch( m_limitsMode )
+    {
+      case WrapperLimitsMode::Warning:
+        GEOS_WARNING( msg );
+        break;
+
+      case WrapperLimitsMode::Error:
+        GEOS_THROW( msg, InputError );
+        break;
+
+      default:
+        GEOS_LOG_RANK_0( "Unimplemented WrapperLimitsMode" );
+        break;


What gives the following in the log?

Suggested change

string const msg = GEOS_FMT( "Value {} for attribute '{}' is outside the allowed range {}.",

value, getDataContext(), m_limits.getRangeStr() );

switch( m_limitsMode )

{

case WrapperLimitsMode::Warning:

GEOS_WARNING( msg );

break;

case WrapperLimitsMode::Error:

GEOS_THROW( msg, InputError );

break;

default:

GEOS_LOG_RANK_0( "Unimplemented WrapperLimitsMode" );

break;

string const msg = GEOS_FMT( "Value {} is outside the allowed range {}.",

value, m_limits.getRangeStr() );

switch( m_limitsMode )

{

case WrapperLimitsMode::Warning:

GEOS_WARNING( msg, getDataContext() );

break;

case WrapperLimitsMode::Error:

GEOS_THROW( msg, InputError, getDataContext() );

break;

default:

GEOS_LOG_RANK_0( "Unimplemented WrapperLimitsMode" );

break;

can you give a stdout sample of the message?

I updated the description

MelReyCG · 2026-06-23T13:45:39Z

+   *                           // default to true.
+   * @endcode
+   */
+  Bound( T v, bool inclusive = true )


That implicit conversion from any double to your type, at this (critical) level of the inclusion hierarchy, may cause the compiler to try undesired casts.
Why not an overload or a different signature for setLimits()?

Causes problems with getDataContext(). See src/coreComponents/dataRepository/Group.cpp:139 "In the Conduit node hierarchy everything begins with 'Problem', we should change it so that the ProblemManager actually uses the root Conduit Node but that will require a full rebaseline."

MelReyCG · 2026-06-25T07:28:00Z

+            string const limitsString = wrapper.getLimitsString();
+            if( !limitsString.empty() )
+            {
+              commentString += GEOS_FMT( " Allowed range: {}", limitsString );
+            }


Maybe add a line break for visibility.

Suggested change

string const limitsString = wrapper.getLimitsString();

if( !limitsString.empty() )

{

commentString += GEOS_FMT( " Allowed range: {}", limitsString );

}

string const limitsString = wrapper.getLimitsString();

if( !limitsString.empty() )

commentString += GEOS_FMT( "\nAllowed range: {}", limitsString );

As it is proposed, the appended string can appear in another sentence:



MelReyCG · 2026-06-25T07:41:31Z

+template< typename T >
+WrapperBound< T > inclusive( T value )
+{
+  return WrapperBound< T >{ value, /*isInclusive*/ true };
+}


I'm afraid those free functions are too global seen their names and scope. A simple solution would be to scope that in a namespace or in the WrapperBound class as static function.

MelReyCG · 2026-06-25T07:43:33Z

+  getMinValue() const
+  {
+    return m_limits.min;
+  }


Is it reading the values or the bound?
In case of reading bound, it can be misleading, I suggest:

Suggested change

getMinValue() const

{

return m_limits.min;

}

getMinBound() const

{

return m_limits.min;

}

kdrienCG added 10 commits May 13, 2026 10:09

(WIP) add min/max limit to Wrapper.hpp

2f08163

Will not compile due to std::optional instantiation (used by m_minValue and m_maxVavlue) for abstract types like PartitionBase

move min/max optionals in a struct instanciated only when T is limitable

45713ea

create is_limitable trait

c07a75a

add limits validation for input values

4c49318

add limits modes to limits

19d2ae8

implement limits modes in validateLimits()

fd14de4

remove setMinValue() and setMaxValue()

32f1586

setLimits() has the same capabilities and should be the only one kept.

add convenience alias is_limitable_v

796bb16

modify outside range message

14bda7b

add inclusive-exclusive properties using a Bound type

581b6d8

kdrienCG self-assigned this May 20, 2026

kdrienCG added the type: feature New feature or request label May 20, 2026

Merge branch 'develop' into feature/kdrienCG/attributeLimitMetadata

2590521

kdrienCG marked this pull request as ready for review May 20, 2026 09:06

kdrienCG requested review from MelReyCG, corbett5, rrsettgast and wrtobin as code owners May 20, 2026 09:06

dkachuma approved these changes May 20, 2026

View reviewed changes

Comment thread src/coreComponents/dataRepository/Wrapper.hpp Outdated

dkachuma approved these changes May 20, 2026

View reviewed changes

Comment thread src/coreComponents/dataRepository/Wrapper.hpp Outdated

kdrienCG added 8 commits May 22, 2026 15:50

support arrays of numbers

7436f3c

set validateLimit() to public

a456aca

add tests

80a9541

add getDataContext() to the limit validation message

65779e4

add range to the limit validation message

54053c7

This part may be refactored, to build the range string somewhere else

set default mode to Error

7d08bd2

move range string creation in the Limits struct

ace25d5

add limits to generated documentation

0f51019

kdrienCG requested review from herve-gross and jhuang2601 as code owners May 29, 2026 13:36

kdrienCG requested a review from OmarDuran as a code owner May 29, 2026 13:36

add GEOS_UNUSED_PARAM to setLimits overload for non-limits

3ccdc0f

kdrienCG mentioned this pull request Jun 22, 2026

feat: add PermeabilitySpecification as an high-level FieldSpecification #4019

Draft

kdrienCG added 2 commits June 22, 2026 11:32

modify BCBakerRelPerm to use limits

931c469

Merge branch 'develop' into feature/kdrienCG/attributeLimitMetadata

7ceb5a6

kdrienCG requested review from bd713, jafranc and joshua-white as code owners June 22, 2026 09:38

Merge branch 'develop' into feature/kdrienCG/attributeLimitMetadata

9d275da

MelReyCG reviewed Jun 22, 2026

View reviewed changes

kdrienCG added 7 commits June 23, 2026 09:11

remove un-needed inclusions

9067eec

rename Bound struct to WrapperBound

600d6cf

rename Limits to WrapperLimits

3de9174

rename file AttributeLimits.hpp to WrapperLimits.hpp

2a0af9f

add doxygen group delimiters for helper methods in WrapperLimits

23d56ce

remove static qualifier

1af0ec5

add check for min <= max when setting a limit

919b359

MelReyCG reviewed Jun 23, 2026

View reviewed changes

kdrienCG added 5 commits June 23, 2026 17:39

remove includes

0022a39

move getDataContext() in logger macros

d0cb744

remove implicit conversions for WrapperBound

7affa36

add schema.xsd.other

98cdb7f

MelReyCG requested changes Jun 25, 2026

View reviewed changes

kdrienCG added 2 commits June 25, 2026 17:56

add new line character to the documentated ranges

340897a

add a namespace for wrapper limits

8a68e85

Uh oh!

Conversation

kdrienCG commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dkachuma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MelReyCG left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kdrienCG commented Jun 23, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kdrienCG commented May 20, 2026 •

edited

Loading